Skip to content

Conversation

@vicLin8712
Copy link
Collaborator

@vicLin8712 vicLin8712 commented Nov 30, 2025

Fix double freed in remove_self_from_waiters()

free() is already invoked in list_remove(). Freeing the same pointer twice can crash the program with an invalid free error or corrupt the heap.

Ensure a single memory-release path by removing the outer free() call.

close #57


Summary by cubic

Removed redundant free in remove_self_from_waiters() to prevent a double-free when removing a waiter. list_remove() already frees the node, avoiding crashes and memory corruption in the mutex waiters list.

Written for commit 4948444. Summary will update automatically on new commits.

cubic-dev-ai[bot]

This comment was marked as resolved.

@jserv
Copy link
Contributor

jserv commented Nov 30, 2025

Can you revisit codebase to eliminate the similar misuse?

@vicLin8712
Copy link
Collaborator Author

Can you revisit codebase to eliminate the similar misuse?

Hi, Jserv,

There is no remaining similar misuse after this PR merged. This PR is the last misuse case.

@jserv jserv requested a review from HeatCrab November 30, 2025 08:31
@jserv
Copy link
Contributor

jserv commented Nov 30, 2025

I defer to @HeatCrab for confirmation.

Copy link
Collaborator

@HeatCrab HeatCrab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I've reviewed the entire codebase for similar misuse patterns and confirmed that this is the only remaining instance that needs to be fixed.
LGTM.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append Close #57 at the end of git commit message.

Copy link
Collaborator

@visitorckw visitorckw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change itself LGTM.

I'm not sure if this description was generated by an LLM, but "prevent doubly function call' sounds a bit strange to me. Calling a function twice isn't an issue, whereas calling free() twice on the same pointer definitely is.

Also, did you omit the user impact from the commit message because it's already described in the issue? I think it's short enough to be included here. The commit message should be self-contained without requiring the reader to check external links.

One last thing: please avoid starting with "This commit". We know it's a commit. Please use the imperative mood instead.

free() is already invoked in list_remove(). Freeing the same pointer twice
can crash the program with an invalid free error or corrupt the heap.

Ensure a single memory-release path by removing the outer free() call.

close sysprog21#57
@vicLin8712
Copy link
Collaborator Author

The code change itself LGTM.

I'm not sure if this description was generated by an LLM, but "prevent doubly function call' sounds a bit strange to me. Calling a function twice isn't an issue, whereas calling free() twice on the same pointer definitely is.

Also, did you omit the user impact from the commit message because it's already described in the issue? I think it's short enough to be included here. The commit message should be self-contained without requiring the reader to check external links.

One last thing: please avoid starting with "This commit". We know it's a commit. Please use the imperative mood instead.

Hi @visitorckw, thanks for your feedback, and I've updated the new commit message.

If you still have further suggestions, please let me know.

@vicLin8712 vicLin8712 requested a review from visitorckw December 1, 2025 02:34
@jserv jserv merged commit b4c2d10 into sysprog21:main Dec 1, 2025
6 checks passed
@jserv
Copy link
Contributor

jserv commented Dec 1, 2025

Thank @vicLin8712 for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants